Skip to content

feat: committor service #366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 77 commits into
base: master
Choose a base branch
from
Open

feat: committor service #366

wants to merge 77 commits into from

Conversation

thlorenz
Copy link
Collaborator

@thlorenz thlorenz commented May 15, 2025

Summary

Adding a full fledged committor service to support the following kind of commits:

  • commits up to 10Kb as long as the size increase per commit is <=1Kb
  • max accounts per bundle is 3 without lookup tables and 12 with lookup tables
  • commit results are persisted into a SQlite database in the ledger directory which makes it
    easy to manually retry them or do this automatically (next committor version will support this)

Details

The following crates were developed
here and integrated
into the validator:

magicblock-rpc-client

  • thin wrapper around the solana rpc client with added features like improved transaction
    signature status checking (including sane retries)
  • lookup table convenience methods
  • multi account fetch with known max limit
  • waiting for slots

magicblock-table-mania

  • a lookup table manager that allows to create, extend and deactivate + close lookup tables
  • the API supports reserving pubkeys which will be added to a table and when they need to be
    used in a transaction the needed table addresses are provided
  • once a pubkey is released this is noted and a table is deactivated and closed on chain once
    all its pubkeys are released

magicblock-committor-program

  • allows the committor service to upload account data to be committed in chunks
  • supports creation/resizing of buffers and filling them with transactions running in parallel
    (out of order is ok)

magicblock-committor-service

  • commits a changeset as fast and efficiently as possible
  • first prefers transactions that don't require lookup tables since those take time to become
    available
  • then prefers args only commits to the use of buffers (which require preparation)

Sample Flow

  1. Validator starts up and reserves common pubkeys (used in all commits) with the committor
    service
  2. Account are cloned and for each account the pubkeys for commits are reserved to prepare the
    lookup tables which we might need as fast as possible
  3. Account(s) commit is scheduled and registered as we did before
  4. The commits are processed via the committor service and the on chain transaction signature
    is provided to the user via logs in a transaction (as we did before, just now we have to wait
    for the commit to complete)

For those commits the committor service ensures that accounts with the same commit (bundle) id
are always processed atomically.

The committor service also picks the best possible strategy to commit each changeset,
preferring speed and then cost.

It also inserts configurable (via code) compute budget instructions to each transaction.

The outcome of each commit is persisted to a database which allows manual (and in the next
version) automated retries of failed commits.
Succeessful commits are also persisted to the database and can be used for diagnostics. In the
future they should be removed since no retry is necessary.

On chain signatures for process-commit/finaliz/undelegate transactions are also persisted to
the database in a separate table.

This table is queried by the validator to log those signatures as part of a transaction that
the user waits for.

Greptile Summary

This PR introduces a comprehensive committor service to the MagicBlock validator for managing account changes and transactions. The implementation includes a new Solana program for handling commits, a service layer for managing commit operations, and integration with lookup tables for transaction optimization.

  • Added magicblock-committor-program that handles chunked account data commits with proper buffer management and security checks
  • Added magicblock-committor-service that orchestrates commit operations with support for bundling, lookup tables, and persistent status tracking via SQLite
  • Added magicblock-table-mania for managing address lookup tables to optimize transaction sizes
  • Added magicblock-rpc-client with enhanced transaction handling and batched account fetching
  • Integrated committor service with existing account management systems while maintaining backward compatibility

thlorenz added 30 commits May 7, 2025 11:10
- at this point schedule commit tests pass with maximum concurrency
Copy link
Contributor

@bmuddha bmuddha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job at documenting the code so thoroughly, left some nits, I have to trust that the logic is sane and the test suite covers all the edge case, as trying to completely understand the entire interplay of all the components would take prohibitive amount of time.

res.await
.map_err(|err| {
// Send request error
AccountClonerError::CommittorSerivceError(format!("{:?}", err))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AccountClonerError::CommittorSerivceError(format!("{:?}", err))
AccountClonerError::CommittorSerivceError(format!("error sending request {err:?}"))

Comment on lines +59 to +71
let max_slot = scheduled_commits
.iter()
.map(|commit| commit.slot)
.max()
.unwrap();
// Safety we just obtained the max slot from the scheduled commits
let ephemeral_blockhash = scheduled_commits
.iter()
.find(|commit| commit.slot == max_slot)
.map(|commit| commit.blockhash)
.unwrap();

changeset.slot = max_slot;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let max_slot = scheduled_commits
.iter()
.map(|commit| commit.slot)
.max()
.unwrap();
// Safety we just obtained the max slot from the scheduled commits
let ephemeral_blockhash = scheduled_commits
.iter()
.find(|commit| commit.slot == max_slot)
.map(|commit| commit.blockhash)
.unwrap();
changeset.slot = max_slot;
let mut changeset = Changeset::default();
let Some(commit) = scheduled_commits.iter().max_by_key(|commit| commit.slot) else {
return Ok(());
};
let max_slot = commit.slot;
let ephemeral_blockhash = commit.blockhash;
changeset.slot = max_slot;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we can safely unwrap()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or keep this and remove empty check above

        if scheduled_commits.is_empty() {
            return Ok(());
        }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree that unwrap is ok. It is very clear that it is a logic (dev) error if we'd crash there (and in that case we'd want to instead of just happily returning Ok)

msg!("Error: {:?}", e);
use CommittorError::*;
let n = match e {
UnableToSerializeChangeSet(_) => 0x69000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are those magic numbers? Some explanatory comments would be helpful

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just us namespacing our errors. I tried to use a starting value that no other programs are using in order to be able to identify errors coming from our program. Any starting number will do here.

/// - *returns* `true` if the pubkey could be reserved
fn reserve(&self, pubkey: &Pubkey) -> bool {
if let Some(count) = self.pubkeys.get(pubkey) {
count.fetch_add(1, Ordering::SeqCst);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SeqCst is a total overkill, Relaxed would be totally correct as we are not synchronizing any other state through this atomic

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure and if we get this wrong then we might end up with stuck lookup tables (which could be expensive for validator operators).
So at a minimal added cost when this method is called I attempt to prevent this.

I'm open to change this to relaxed if we ensure there is no way for race conditions.

fn release(&self, pubkey: &Pubkey) -> bool {
if let Some(count) = self.pubkeys.get(pubkey) {
count
.fetch_update(Ordering::SeqCst, Ordering::SeqCst, |x| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, SeqCst is an overkill, since no other state is synchronized

fn has_reservations(&self) -> bool {
self.pubkeys
.values()
.any(|rc_pubkey| rc_pubkey.load(Ordering::SeqCst) > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, Ordering::Relaxed is enough

})
}

/// Returns `true` if the we requested to deactivate this table.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Returns `true` if the we requested to deactivate this table.
/// Returns `true` if we have already requested to deactivate this table.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to get more wordy here (the already is basically a flller IMO).


#[derive(Clone)]
pub struct TableMania {
pub active_tables: Arc<RwLock<Vec<LookupTableRc>>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess some research was made to justify the usage of async synchronization primitives, i.e. they will be used in highly concurrent environments and with lock guards being held across await points, if not, then their use is unjustified.

Copy link
Contributor

@taco-paco taco-paco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments and questions.

@@ -66,6 +70,20 @@ pub enum AccountClonerUnclonableReason {
DelegatedAccountsNotClonedWhileHydrating,
}

pub async fn map_committor_request_result<T>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would propose to use imp From<> for AccountClonerError to get rid of this function. It adds more indirection where code is really simple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to have this be a very explicit conversion as we also include some more information on top.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like ChangesetBundles::from would be explicit enough

"Committor service persists to: {}",
committor_persist_path.display()
);
let committor_service = Arc::new(CommittorService::try_start(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That goes out from usual flow of services. Would be good to start it from Validator::start along with other services

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly specific services have to be started only after some other services are started already.
Here we start all those services together in the right order.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt it was due to separation: instance creation, start work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For ease of reading I think Actor shall be moved to its personal file

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree as the actor is just the internal impl of the API interface and both are tightly coupled, thus makes sense to have them in the same module.

RpcClientError(#[from] solana_rpc_client_api::client_error::Error),

#[error("Error getting blockhash: {0} ({0:?})")]
GetLatestBlockhash(solana_rpc_client_api::client_error::Error),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those errors intended to be handled? Not sure we need error per rpc entrypoint. If we want to have extra context I'd suggest logging instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a more core lib (which this is) you don't want to just log, but provide detailed errors ror the upper level logic to handle, i.e. it might handle a missing blockhash differently than a deserialization error.
If we don't provide different errors for those cases then this is not possible.

};

let storing = remaining[..storing_len].to_vec();
let stored = table
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make this work without new allocations at all.

We just pass &mut remaining into extend_respecting_capacity, which would pop from the back pubkeys it needs


/// How many process and commit buffer instructions fit into a single transaction
#[allow(unused)] // serves as documentation as well
pub(crate) const MAX_PROCESS_PER_TX: u8 = 3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How were those consts determined? I think also worth adding this information as comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See tests below in this file

/// when using address lookup tables but not including the buffer account in the
/// lookup table
#[allow(unused)] // serves as documentation as well
pub(crate) const MAX_PROCESS_PER_TX_USING_LOOKUP: u8 = 12;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How were those consts determined? I think also worth adding this information as comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See tests below in this file

.first()
.map(|(_, acc)| acc.bundle_id())
.unwrap_or_default();
Err(CommittorServiceError::CouldNotFindCommitStrategyForBundle(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like a very critical error. It pretty much breaks a contract that validator has with user - you can commit and undelegate your state, With this it's not true anymore.
it actually shall be slashable when validator doesn't commits or undelegates your state. Now it will be hard to handle this.

I think our magicblock-program shall have some surface-level check to reject impossible commits. Like if number of accounts to commit somehow > 32*256 (possible with lookup tables) we panic. or even > 30 * 256. Also we could pretty easily calculate overall sizes of committed accounts and make some estimates.

Thesis is: if something made it into scheduled_commits it has to be commited, otherwise validator is slashed. So here we need to panic really

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea. However our validator needs to keep running in these cases and there is no direct way to communicate this back to the user. Keep in mind that it is the program that schedules commits, so there is no way this is the validator's fault if we exceed limits. Instead we fail those commits and keep an entry in our DB which we can then query in order to provide feedback to program authors about those violations. For now manually, but possibly we may provide a query interface for validator operators or even users as well (i.e. as part of a dashboard).

undelegation_requested: commit.request_undelegation,
});
committees.push((
commit.id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need in commit.id here. In committees vector the all belong to same id

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need that id further down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't find it, maybe missed it. Which part do you refer to?

lamports: committee.account_data.lamports(),
data: committee.account_data.data().to_vec(),
owner: committee.owner,
bundle_id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bundle_id,
commit.id,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bundle_id can removed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever clean the entries up?


pub fn ensure_processed() -> Self {
Self::SendAndConfirm {
wait_for_blockhash_to_become_valid: Some(Duration::from_millis(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we need all of the Options in MagicBlockSendTransactionConfig if all of those are defined, or defaulted to some value during runtime.
MagicBlockSendTransactionConfig can get rid from all the Options

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This library is intended to be used in more cases in the future (than just from the committer service). Therefore I made them configurable.

wait_for_commitment_level
{
let now = Instant::now();
let check_for_commitment_interval = check_for_commitment_interval
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Value can be defaulted to this on construction if not set.

Copy link
Collaborator Author

@thlorenz thlorenz Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to do this at last possible moment and closer to the code that uses it.

.map(|account| account.bundle_id)
.collect::<HashSet<_>>()
{
let bundle_signatures = match changeset_committor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we return all of the signatures as result of commit_changeset?

  1. That would allow us to clean entries right away from persister.
  2. At this point all of the signatures already available, this call just reads them from db.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we could, but I don't want to introduce two sources of truth.
The idea is to return minimal info as part of the result and only if more details are needed do we query the DB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be 1 source of truth - returned value.

I see 2 problems with the other way around:

  1. we perform I/O for something that is result of function
  2. Persister isn't cleaned

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source of truth is that is in the DB, we don't have to also return it IMO.

I/O is only needed if something goes wrong.

Cleaning of persister (removing rows for successful commits) will be added in a follow up once we saw everything working as expected.


// Allow the committer service to reserve pubkeys in lookup tables
// that could be needed when we commit this account
map_committor_request_result(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like all pubkeys will be in some lookup table, even when they're not needed to. For example, do we need this in case when we can commit using just arguments?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we create tables only when necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know this ahead of time, but need to prepare for the case where we do need them in order to save time in that case. This was a nice optimization idea by @bmuddha

Copy link
Contributor

@taco-paco taco-paco Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a valid point, but the other side is that validator pays when this may not be needed. I don't think waiting for txs to land in this case is so critical.

But it depends what we want to opimize at the end

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We optimize for fast commits

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with TableMania if validator is restarted?
It looks like if validator is restarted we would start from scratch, creating lookup tables for pubkeys, that actually are already in some lookup table

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great observation. Remember that we derive the tablemania pubkeys deterministically which allows to find old tables and close them later to refund cost to the validator.
The alternative would be to delay validator stop for a few slots until we were able to deactivate and then close all tables (not a good solution IMO).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the garbage collector TableMania? Unless I miss something, now it looks like:

  1. once some account per pubkey is cloned, we extend/create a new table, or do nothing.
  2. once the addresses in table all undelegated it can be cleaned up.

From here I don't see why we need garbage collector:

  1. Ideally use tables only when its required for successful commit. Insert/extend/do nothing(if present) as per current logic
  2. Once the last key in some table is released - deactivate table right away.

No need for garbage collector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to first deactivate the table (checking in the background if that is possible).
Then we need to wait for a few slots until we can close it.

We cannot do this as part of the flow which needs to be fast.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the part of asynchronous deactivation is fine with me) By garbage collector I also implied our reference counting and all that logic. It isn't needed in this flow. Even in current implementation the tables will be marked deactivated. so we can just completely remove garbage collection based on reference counting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is that not needed? We need to know which pubkeys are still in use in order to know which tables can be deactivated and later closed. Keep in mind that we want to clean up the lookup tables as soon as possible, i.e. when we know that certain accounts were undelegated and thus will no longer be committed until they are delegated and cloned again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest keeping it ./programs folder

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That folder is for programs that are part of our ephemeral validator. This program will be deployed on dev/mainnet, so it is different (more like the delegation program which is even in a separate repo)

@@ -111,8 +102,13 @@ magicblock-accounts-api = { path = "./magicblock-accounts-api" }
magicblock-accounts-db = { path = "./magicblock-accounts-db" }
magicblock-api = { path = "./magicblock-api" }
magicblock-bank = { path = "./magicblock-bank" }
magicblock-committor-program = { path = "./magicblock-committor-program", features = [
Copy link
Contributor

@taco-paco taco-paco May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: we now have services ending with committor and committer, like RemoteAccountCommitter.
committor must be a typo

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, everything in the committor-service is committor. RemoteAccountCommitter existed before.
It's a solana thing as we are buildooors. I'm trying to keep our codebase fun :)

@GabrielePicco GabrielePicco requested a review from Copilot June 4, 2025 07:34
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a full committor service to support efficient, atomic account commits with integration across multiple modules including the new committor program, service, and lookup table management. Key changes include:

  • Adding new crates (magicblock-committor-program and magicblock-committor-service) to handle commit data, buffer management, and persistence via SQLite.
  • Integrating the committor service into the validator and account processing pipelines by updating the APIs and tests.
  • Updating error handling and dependency configurations across several modules to support the new commit flows.

Reviewed Changes

Copilot reviewed 118 out of 118 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
magicblock-committor-program/src/consts.rs Introduces new constant definitions supporting commit limits.
magicblock-api/src/tickers.rs Integrates committor service references in slot ticker processing.
magicblock-api/src/magic_validator.rs Incorporates committor service for reserve and commit operations.
magicblock-api/src/errors.rs Adds new error variants including validator funding and committor.
magicblock-accounts/* Updates signatures and test setups to propagate changeset_committor.
magicblock-account-cloner/* Adjusts account cloning and error mapping to use changeset_committor.
Cargo.toml (workspace root and sub-crates) Updates dependencies and workspace members for new committor crates.
Comments suppressed due to low confidence (1)

magicblock-accounts/src/remote_scheduled_commits_processor.rs:216

  • [nitpick] The process_changeset function contains several nested operations; consider refactoring parts of its logic into smaller helper functions to improve readability and maintainability.
    fn process_changeset<CC: ChangesetCommittor>(...) {

thlorenz added 3 commits June 4, 2025 14:53
* master:
  feat: update mdp dependency + tests
  fix: use mutiple connections for ws subscriptions (#365)
  feat: allow for multiple ws endpoints in remote config (#372)
  Raise mdp version + update artifacts & improve error logging (#368)
CommitBundleStrategy::try_from((bundle, finalize))?;
match commit_strategy {
CommitBundleStrategy::Args(bundle) => {
add_to_changeset(
Copy link
Contributor

@taco-paco taco-paco Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it safe to append on new iteration to same args_changeset?

Say we have 2 bundles:
Iteration 1 - we get CommitBundleStrategy::Args and push to args_changeset.
Iteration 2 - we get CommitBundleStrategy::Args and push to args_changeset.

Now the issue it that if bundle in Iteration 1 and 2 separately fit into args, doesn't mean they both will.

Looking into commit_changeset_using_args it doesn't handle them on bundle levels but tries create a single tx with data from 2 Iterations

pubkey: Pubkey,
account: T,
) -> bool {
self.accounts.insert(pubkey, account.into()).is_some()
Copy link
Contributor

@taco-paco taco-paco Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// If it already exists, it will be replaced, thus the caller needs

I think that could be an issue, assume actions.
Imagine:

  1. Bob makes a CommitWithActions with some accounts [acc1, acc2], and actions [a1, a2]. commit_id = 1
  2. Bob makes a CommitWithActions with some accounts [acc2, acc3], and actions [a3, a4]. commit_id = 2

At this point we would replaced acc2 with commit_id = 1, to acc2 with commit_id = 2. As result for commit_id/bundle_id = 1 we would commit [acc1] with actions [a1, a2], when user expected both of them to be commited, and action could be connected to state of acc2.

Shouldn't we keep all accounts per commit_id, and not replace them?
like:
First [acc1, acc2], and actions [a1, a2], then [acc2, acc3], and actions [a3, a4]
cc @GabrielePicco

thlorenz and others added 14 commits June 18, 2025 16:06
* master:
  Optimizing column entries bookkeeping (#393)
  Update README.md (#396)
  Revert to using separate tokio runtime instance for RPC (#395)
  feat: fix release workflow (#392)
  Compaction V1: Manual compaction (#383)
  release: v0.1.3 (#391)
  Log validator version on startup (#390)
  Update README.md (#384)
  PubSub redesign from broadcast to multicast  (#373)
* master:
  release: v0.1.7 (#409)
  Feat: simpler gasless implementation (#406)
  perf: run ensure accounts concurrently
  feat: account hydration on background task (#407)
  Revert "feat: Cleaner gasless implementation" (#405)
  feat: Cleaner gasless implementation (#360)
  fix: cleanup slots on non-fresh ledger (#404)
  release: v0.1.5 (#403)
  fix: properly init geyser plugin for rpc and use valid dummy Library (#402)
  release: v0.1.4 (#401)
  fix: remove grpc plugin altogether from geyser manager (#400)
  Patch tests from failing on subscription due to rate limits (#386)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants